Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Confirm less is link before using it as such #820

Merged
merged 2 commits into from
Sep 11, 2024
Merged

Confirm less is link before using it as such #820

merged 2 commits into from
Sep 11, 2024

Conversation

piurafunk
Copy link
Contributor

The PHP docs of readlink() indicate that if the path provided is not a link, it fails.

Note: The function fails if path is not a symlink, except on Windows, where the normalized path will be returned.

I am seeing this in Docker with PHP FPM image, with Alpine 3.20.3, PHP 8.2.23:

65aefc8b7840:/var/www/html$ php artisan tinker

   ErrorException 

  readlink(): Invalid argument

  at vendor/psy/psysh/src/Configuration.php:1266
    1262▕                 // n.b. The busybox less implementation is a bit broken, so
    1263▕                 // let's not use it by default.
    1264▕                 //
    1265▕                 // See https://github.com/bobthecow/psysh/issues/778
  ➜ 1266▕                 $link = @\readlink($less);
    1267▕                 if ($link !== false && \strpos($link, 'busybox') !== false) {
    1268▕                     return false;
    1269▕                 }
    1270▕ 

      +16 vendor frames 
  17  artisan:31
      Illuminate\Foundation\Console\Kernel::handle(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))

@piurafunk
Confirm less is link before using it as such
f3dc77e 
The PHP [docs](https://www.php.net/manual/en/function.readlink.php) of `readlink()` indicate that if the path provided is not a link, it fails.

> Note: The function fails if path is not a symlink, except on Windows, where the normalized path will be returned.
@piurafunk
Copy link
Contributor Author

Just discovered that the reason less is not a symlink in my environment is because we install it specifically in our Dockerfile:

RUN apk add --no-cache \
    ...
    less \
    ...
    ;

which then clobbers the symlink:

65aefc8b7840:/var/www/html$ ls -lah /usr/bin/less
-rwxr-xr-x    1 root     root      167.9K Apr 17 02:57 /usr/bin/less

Would still love this to be merged, though; it'll help stabilize in different environments.

@bobthecow
Copy link
Owner

that @ is meant to suppress the error. something is overeagerly changing them into ErrorExceptions :)

@piurafunk
Copy link
Contributor Author

Yes, I imagine that is Laravel. In combination with PHP 8, the @ symbol reports it's error level, see docs. Which causes Laravel to throw a new ErrorException.

It seems like defensive programming to first check if it is a link before treating it as such. Do you agree with that?

@bobthecow
Copy link
Owner

yeah, it's reasonable to check first.

if we're going to do that, we should wrap this whole thing in a conditional block, rather than using a ternary to fall back to the actual less path. this line of code is trying to say "if less is a symlink, and it includes busybox in the actual path, then…"

@piurafunk
Copy link
Contributor Author

I've updated that in 0c59c2d.

@bobthecow bobthecow merged commit f252298 into bobthecow:main Sep 11, 2024
27 checks passed
@bobthecow
Copy link
Owner

Thanks!

@piurafunk piurafunk deleted the patch-1 branch September 18, 2024 14:25
@piurafunk
Copy link
Contributor Author

@bobthecow How soon/frequently do you do releases? I'd love to upgrade to get this change in our app.

@bobthecow
Copy link
Owner

There's no set cadence, but it's probably about time. I'll get this released in the next few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@piurafunk @bobthecow